feat(helm)!: restructure values and deliver config via NGROK_OPERATOR…#829
feat(helm)!: restructure values and deliver config via NGROK_OPERATOR…#829jonstacks wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis change adds environment-backed CLI defaults, restructures Helm values under shared and component-scoped keys, moves component runtime config into ConfigMaps plus env injection, updates chart templates and tests to the new paths, and refreshes migration and uninstall documentation. ChangesConfiguration model migration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~100 minutes Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
github-actions
Bot
added
documentation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #829 +/- ##
==========================================
+ Coverage 52.30% 52.33% +0.02%
==========================================
Files 103 104 +1
Lines 11379 11413 +34
==========================================
+ Hits 5952 5973 +21
- Misses 4975 4985 +10
- Partials 452 455 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…_* env vars Restructure the Helm values tree around shared platform config (ngrok.*), feature flags (features.*), and per-component sections (apiManager, agent, bindingsForwarder), implementing Option 2 of the helm values redesign. App config now reaches the operator binaries the same way Argo CD does it: every app-config CLI flag reads an NGROK_OPERATOR_* environment variable as its default (CLI flag > env var > built-in default), and the chart injects those variables from per-component ConfigMaps via configMapKeyRef entries with optional: true. Per-component overrides are handled at render time by merging <component>.config over the shared config into that component's ConfigMap. Pods roll on config changes via a checksum/config annotation. Also: - Add a values guard that fails the render with an old->new mapping when pre-v1 values are present, so they are never silently ignored - Wire commonAnnotations into every chart resource (was declared but unused) - Fix agent/bindings-forwarder ServiceAccount component labels - Update helm unittest suites, chainsaw fixtures, deploy targets, specs, migration guide, README/schema, and manifest bundle Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Jonathan Stacks <jonstacks@users.noreply.github.com>
jonstacks
force-pushed
the
helm-values-refactor
branch
from
5f14e5c to
8c7fdd9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
specs/migration-v1.md (1)
30-57: ⚡ Quick winClarify
commonAnnotationsbehavior change in migration guide.Line 54 lists
commonAnnotationsunder "Unchanged" (value path correct), but the PR objective states these are now "wired into all chart resources (previously declared but unused)". The path is unchanged, but the runtime behavior is new. Users migrating might assume no change is needed; explicitly noting that the path stays the same but the feature is now active would reduce confusion.Consider clarifying:
commonAnnotations(path unchanged, but now wired into all resources).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/migration-v1.md` around lines 30 - 57, The migration guide lists commonAnnotations in the "Unchanged" section (around line 54), but this is misleading because while the configuration path remains the same, the runtime behavior has changed—commonAnnotations is now actively wired into all chart resources instead of being previously unused and ignored. Update the commonAnnotations entry in the "Unchanged" section to clarify that the path is unchanged but the feature behavior is new, using wording similar to: commonAnnotations (path unchanged, but now wired into all resources). This prevents users from assuming no action or consideration is needed when migrating.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@helm/ngrok-operator/templates/_helpers.tpl`:
- Around line 135-141: The ngrok-operator.kvPairs template function iterates
over map keys in non-deterministic order, causing identical input values to
render differently and trigger unnecessary ConfigMap checksum changes and pod
rollouts. To fix this, modify the range operation in the ngrok-operator.kvPairs
function to sort the keys before iterating over them, ensuring the output order
is always consistent regardless of the input map's internal ordering. This will
guarantee deterministic rendering of the key-value pairs used in ConfigMap
configurations.
In `@helm/ngrok-operator/templates/rbac/crd-access/domain-editor.yaml`:
- Around line 9-10: In the domain-editor.yaml file where the annotations
variable is defined, reverse the order of the merge arguments for the
crdAccessRoles and commonAnnotations to ensure correct precedence. Currently,
commonAnnotations is merged last (giving it higher priority), but it should be
merged first with crdAccessRoles merged afterward. This ensures that
role-specific annotations in crdAccessRoles override the common base annotations
when keys conflict, following the expected specific-over-general precedence
pattern.
In `@helm/ngrok-operator/values.schema.json`:
- Around line 97-110: The schema validation for the "level", "format", and
"stacktraceLevel" fields documents their allowed values in descriptions but does
not enforce them with constraints, allowing invalid configurations to pass
validation. Add an `enum` constraint to each field: for "level" add enum with
values debug, info, and error; for "format" add enum with values console and
json; for "stacktraceLevel" add enum with values info and error. Ensure these
enum constraints match the allowed values documented in each field's
description. Also apply the same pattern to the similar fields mentioned at
lines 234-243.
- Around line 350-354: The schema default for the maxUnavailable field within
apiManager.podDisruptionBudget is set to an empty string ("\"\""), but the
actual default in values.yaml is "1". Update the default value in the schema
file for the maxUnavailable property to match the chart's actual default of "1"
to ensure consistency between the schema documentation and the runtime
configuration.
---
Nitpick comments:
In `@specs/migration-v1.md`:
- Around line 30-57: The migration guide lists commonAnnotations in the
"Unchanged" section (around line 54), but this is misleading because while the
configuration path remains the same, the runtime behavior has
changed—commonAnnotations is now actively wired into all chart resources instead
of being previously unused and ignored. Update the commonAnnotations entry in
the "Unchanged" section to clarify that the path is unchanged but the feature
behavior is new, using wording similar to: commonAnnotations (path unchanged,
but now wired into all resources). This prevents users from assuming no action
or consideration is needed when migrating.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c6ca502f-2603-4d58-b534-ecaf5ec433cc
⛔ Files ignored due to path filters (10)
helm/ngrok-operator/tests/__snapshot__/agent-config-cm_test.yaml.snapis excluded by!**/*.snaphelm/ngrok-operator/tests/__snapshot__/binding-configuration_test.yaml.snapis excluded by!**/*.snaphelm/ngrok-operator/tests/agent/__snapshot__/deployment_test.yaml.snapis excluded by!**/*.snaphelm/ngrok-operator/tests/agent/__snapshot__/service-account_test.yaml.snapis excluded by!**/*.snaphelm/ngrok-operator/tests/agent/__snapshot__/serviceaccount_test.yaml.snapis excluded by!**/*.snaphelm/ngrok-operator/tests/api-manager/__snapshot__/configmap_test.yaml.snapis excluded by!**/*.snaphelm/ngrok-operator/tests/api-manager/__snapshot__/deployment_test.yaml.snapis excluded by!**/*.snaphelm/ngrok-operator/tests/bindings-forwarder/__snapshot__/deployment_test.yaml.snapis excluded by!**/*.snaphelm/ngrok-operator/tests/bindings-forwarder/__snapshot__/service-account_test.yaml.snapis excluded by!**/*.snaphelm/ngrok-operator/tests/bindings-forwarder/__snapshot__/serviceaccount_test.yaml.snapis excluded by!**/*.snap
📒 Files selected for processing (88)
cmd/agent-manager.gocmd/api-manager.gocmd/bindings-forwarder-manager.godocs/uninstall.mdgo.modhelm/ngrok-operator/README.mdhelm/ngrok-operator/templates/NOTES.txthelm/ngrok-operator/templates/_helpers.tplhelm/ngrok-operator/templates/agent/configmap.yamlhelm/ngrok-operator/templates/agent/deployment.yamlhelm/ngrok-operator/templates/agent/release-namespace-role.yamlhelm/ngrok-operator/templates/agent/role.yamlhelm/ngrok-operator/templates/agent/rolebinding.yamlhelm/ngrok-operator/templates/agent/serviceaccount.yamlhelm/ngrok-operator/templates/api-manager/bindings-cluster-role.yamlhelm/ngrok-operator/templates/api-manager/configmap.yamlhelm/ngrok-operator/templates/api-manager/deployment.yamlhelm/ngrok-operator/templates/api-manager/leader-election-role.yamlhelm/ngrok-operator/templates/api-manager/pdb.yamlhelm/ngrok-operator/templates/api-manager/release-namespace-role.yamlhelm/ngrok-operator/templates/api-manager/role.yamlhelm/ngrok-operator/templates/api-manager/rolebinding.yamlhelm/ngrok-operator/templates/api-manager/serviceaccount.yamlhelm/ngrok-operator/templates/bindings-forwarder/configmap.yamlhelm/ngrok-operator/templates/bindings-forwarder/deployment.yamlhelm/ngrok-operator/templates/bindings-forwarder/role.yamlhelm/ngrok-operator/templates/bindings-forwarder/serviceaccount.yamlhelm/ngrok-operator/templates/cleanup-hook/job.yamlhelm/ngrok-operator/templates/cleanup-hook/rbac.yamlhelm/ngrok-operator/templates/credentials-secret.yamlhelm/ngrok-operator/templates/ingress-class.yamlhelm/ngrok-operator/templates/rbac/crd-access/agentendpoint-editor.yamlhelm/ngrok-operator/templates/rbac/crd-access/agentendpoint-viewer.yamlhelm/ngrok-operator/templates/rbac/crd-access/boundendpoint-editor.yamlhelm/ngrok-operator/templates/rbac/crd-access/boundendpoint-viewer.yamlhelm/ngrok-operator/templates/rbac/crd-access/cloudendpoint-editor.yamlhelm/ngrok-operator/templates/rbac/crd-access/cloudendpoint-viewer.yamlhelm/ngrok-operator/templates/rbac/crd-access/domain-editor.yamlhelm/ngrok-operator/templates/rbac/crd-access/domain-viewer.yamlhelm/ngrok-operator/templates/rbac/crd-access/ippolicy-editor.yamlhelm/ngrok-operator/templates/rbac/crd-access/ippolicy-viewer.yamlhelm/ngrok-operator/templates/rbac/crd-access/kubernetesoperator-editor.yamlhelm/ngrok-operator/templates/rbac/crd-access/kubernetesoperator-viewer.yamlhelm/ngrok-operator/templates/rbac/crd-access/ngroktrafficpolicy-editor.yamlhelm/ngrok-operator/templates/rbac/crd-access/ngroktrafficpolicy-viewer.yamlhelm/ngrok-operator/templates/validate-values.yamlhelm/ngrok-operator/tests/agent/configmap_test.yamlhelm/ngrok-operator/tests/agent/deployment_test.yamlhelm/ngrok-operator/tests/agent/rbac_test.yamlhelm/ngrok-operator/tests/api-manager/configmap_test.yamlhelm/ngrok-operator/tests/api-manager/deployment_test.yamlhelm/ngrok-operator/tests/api-manager/pdb_test.yamlhelm/ngrok-operator/tests/api-manager/rbac_test.yamlhelm/ngrok-operator/tests/api-manager/serviceaccount_test.yamlhelm/ngrok-operator/tests/bindings-forwarder/configmap_test.yamlhelm/ngrok-operator/tests/bindings-forwarder/deployment_test.yamlhelm/ngrok-operator/tests/bindings-forwarder/rbac_test.yamlhelm/ngrok-operator/tests/bindings-forwarder/serviceaccount_test.yamlhelm/ngrok-operator/tests/ingress-class_test.yamlhelm/ngrok-operator/tests/validate-values_test.yamlhelm/ngrok-operator/values.schema.jsonhelm/ngrok-operator/values.yamlinternal/env/env.gointernal/env/env_test.gomanifest-bundle.yamlspecs/helm/agent.mdspecs/helm/bindings-forwarder.mdspecs/helm/common.mdspecs/helm/operator.mdspecs/migration-v1.mdtests/chainsaw-uninstall/_fixtures/values-base.yamltests/chainsaw-uninstall/_fixtures/values-bindings-enabled.yamltests/chainsaw-uninstall/bindings-delete-policy/values.yamltests/chainsaw-uninstall/bindings-retain-policy/values.yamltests/chainsaw-uninstall/delete-policy-bundled-crds/values.yamltests/chainsaw-uninstall/delete-policy-separate-crds/values.yamltests/chainsaw-uninstall/multi-ingressclass-delete-policy/values-operator-a.yamltests/chainsaw-uninstall/multi-ingressclass-delete-policy/values-operator-b.yamltests/chainsaw-uninstall/multi-ingressclass-retain-policy/values-operator-a.yamltests/chainsaw-uninstall/multi-ingressclass-retain-policy/values-operator-b.yamltests/chainsaw-uninstall/multi-ns-delete-policy/values-operator-a.yamltests/chainsaw-uninstall/multi-ns-delete-policy/values-operator-b.yamltests/chainsaw-uninstall/multi-ns-retain-policy/values-operator-a.yamltests/chainsaw-uninstall/multi-ns-retain-policy/values-operator-b.yamltests/chainsaw-uninstall/retain-policy-bundled-crds/values.yamltests/chainsaw-uninstall/retain-policy-separate-crds/values.yamltools/make/_common.mktools/make/deploy.mk
| {{- define "ngrok-operator.kvPairs" -}} | ||
| {{- $pairs := list -}} | ||
| {{- range $key, $value := . -}} | ||
| {{- $pairs = append $pairs (printf "%s=%v" $key $value) -}} | ||
| {{- end -}} | ||
| {{- $pairs | join "," -}} | ||
| {{- end -}} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify current helper body and confirm whether map keys are sorted.
rg -n -A10 -B2 'define "ngrok-operator.kvPairs"' helm/ngrok-operator/templates/_helpers.tpl
# Expected after fix: usage of `keys` and `sortAlpha` before indexing values.Repository: ngrok/ngrok-operator
Length of output: 530
🏁 Script executed:
rg -n 'kvPairs' helm/ngrok-operator/templates/ -A3 -B3Repository: ngrok/ngrok-operator
Length of output: 3292
🌐 Web query:
Helm sprig functions sortAlpha keys
💡 Result:
In Helm templates, dictionary keys are inherently unordered [1][2]. Because iterating over a dictionary in a template can result in non-deterministic output, it is a best practice to extract the keys into a list and sort them using the sprig function sortAlpha [3][4][2]. The sortAlpha function sorts a list of strings into alphabetical (lexicographical) order [5][6]. It does not modify the list in place but returns a new, sorted copy [6]. To iterate over a dictionary in a deterministic order, you can use the following pattern: {{- range $key:= keys.Values.myDict | sortAlpha }} {{
Citations:
- 1: https://github.com/helm/helm-www/blob/main/docs/chart_template_guide/function_list.mdx
- 2: http://masterminds.github.io/sprig/dicts.html
- 3: non-deterministic ordering from "range" results in unnecessary deployments helm/helm#4375
- 4: https://oneuptime.com/blog/post/2026-01-17-helm-template-functions-pipelines/view
- 5: http://masterminds.github.io/sprig/string_slice.html
- 6: https://github.com/Masterminds/sprig/blob/master/docs/string_slice.md
- 7: https://master--helm-dev.netlify.app/docs/chart_template_guide/function_list/
🏁 Script executed:
# Verify that 'index' function is available in Helm/Sprig
web_search "Helm sprig index function map"Repository: ngrok/ngrok-operator
Length of output: 115
🌐 Web query:
Helm Sprig index function dictionary retrieval
💡 Result:
In Helm templates, the index function is the standard, built-in Go template function used to retrieve values from a dictionary (map) using a dynamic key [1][2][3]. Although the Helm documentation often emphasizes its use for lists (slices), it is fully supported for dictionary retrieval [4][5][6]. Basic Usage To retrieve a value from a dictionary using a dynamic key, use the following syntax [3]: {{ index.MapVariable "keyName" }} In this example,.MapVariable is the dictionary and "keyName" is the key you wish to access. You can also pass a variable instead of a string literal for the key [3]: {{- $myKey:= "someKey" -}} {{ index.Values.myMap $myKey }} Nested Retrieval The index function supports multi-level lookups by passing additional arguments [7][5][2]. For example, to access a nested value: {{ index.Values "level1" "level2" "targetKey" }} Handling Missing Keys If the key does not exist, the index function returns nil [8][2]. This can cause template execution to fail if you attempt to perform further operations (like accessing sub-keys) on that nil value [8][2]. A common pattern to handle this is to use the default function with an empty dictionary (dict) to ensure a safe fallback [8][2]: {{- $subDict:= index.Values "maybeMissing" | default dict -}} {{- $value:= index $subDict "targetKey" -}} Alternative Functions While index is the primary tool for dynamic retrieval, the Sprig library provides additional dictionary functions that may be useful depending on your needs: - get: Similar to index, but specifically designed for dictionaries [9]. - dig: Useful for traversing nested dictionaries safely, as it allows for a default value if any key in the path is missing [9]. - pluck: Useful if you have a key and multiple dictionaries and want to retrieve the value from whichever dictionary contains that key [9][7].
Citations:
- 1: https://stackoverflow.com/questions/63490926/extracting-value-dynamically-from-dictionary-on-helm2
- 2: https://stackoverflow.com/questions/75060796/helm-using-argument-in-template-to-check-if-value-exists-in-values-yaml
- 3: https://stackoverflow.com/questions/74274868/how-to-use-val-to-get-value-from-map-in-helm-for-kubernetes
- 4: https://github.com/helm/helm-www/blob/main/docs/chart_template_guide/function_list.mdx
- 5: https://www.bookstack.cn/read/helm-3.13.0-en/a18ab73db58bb6f4.md
- 6: How to use value as other value's key? helm/helm#7267
- 7: https://stackoverflow.com/questions/77166935/helm-templating-index-for-subkeys
- 8: https://stackoverflow.com/questions/61660245/how-to-use-default-if-index-does-not-exists
- 9: http://masterminds.github.io/sprig/dicts.html
Sort keys in ngrok-operator.kvPairs to keep rendered config deterministic.
Map iteration here is non-deterministic, causing identical values to render in different orders and trigger unnecessary checksum/config changes and pod rollouts. This happens at lines 172, 189, and 190 where the output feeds into ConfigMap configurations.
Suggested fix
{{- define "ngrok-operator.kvPairs" -}}
{{- $pairs := list -}}
-{{- range $key, $value := . -}}
-{{- $pairs = append $pairs (printf "%s=%v" $key $value) -}}
+{{- $m := . -}}
+{{- range $key := keys $m | sortAlpha -}}
+{{- $pairs = append $pairs (printf "%s=%v" $key (index $m $key)) -}}
{{- end -}}
{{- $pairs | join "," -}}
{{- end -}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{- define "ngrok-operator.kvPairs" -}} | |
| {{- $pairs := list -}} | |
| {{- range $key, $value := . -}} | |
| {{- $pairs = append $pairs (printf "%s=%v" $key $value) -}} | |
| {{- end -}} | |
| {{- $pairs | join "," -}} | |
| {{- end -}} | |
| {{- define "ngrok-operator.kvPairs" -}} | |
| {{- $pairs := list -}} | |
| {{- $m := . -}} | |
| {{- range $key := keys $m | sortAlpha -}} | |
| {{- $pairs = append $pairs (printf "%s=%v" $key (index $m $key)) -}} | |
| {{- end -}} | |
| {{- $pairs | join "," -}} | |
| {{- end -}} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@helm/ngrok-operator/templates/_helpers.tpl` around lines 135 - 141, The
ngrok-operator.kvPairs template function iterates over map keys in
non-deterministic order, causing identical input values to render differently
and trigger unnecessary ConfigMap checksum changes and pod rollouts. To fix
this, modify the range operation in the ngrok-operator.kvPairs function to sort
the keys before iterating over them, ensuring the output order is always
consistent regardless of the input map's internal ordering. This will guarantee
deterministic rendering of the key-value pairs used in ConfigMap configurations.
| {{- $annotations := merge (dict) (.Values.crdAccessRoles.annotations | default dict) (.Values.commonAnnotations | default dict) }} | ||
| {{- with $annotations }} |
There was a problem hiding this comment.
Reverse the merge order for correct annotation precedence.
The current merge order causes commonAnnotations to override crdAccessRoles.annotations when keys conflict. This inverts the expected specific-over-general precedence: role-specific annotations should override common base annotations, not the other way around.
In Helm's merge, later arguments take precedence. The correct order should merge common annotations first, then role-specific annotations:
🔧 Proposed fix
- {{- $annotations := merge (dict) (.Values.crdAccessRoles.annotations | default dict) (.Values.commonAnnotations | default dict) }}
+ {{- $annotations := merge (dict) (.Values.commonAnnotations | default dict) (.Values.crdAccessRoles.annotations | default dict) }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{- $annotations := merge (dict) (.Values.crdAccessRoles.annotations | default dict) (.Values.commonAnnotations | default dict) }} | |
| {{- with $annotations }} | |
| {{- $annotations := merge (dict) (.Values.commonAnnotations | default dict) (.Values.crdAccessRoles.annotations | default dict) }} | |
| {{- with $annotations }} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@helm/ngrok-operator/templates/rbac/crd-access/domain-editor.yaml` around
lines 9 - 10, In the domain-editor.yaml file where the annotations variable is
defined, reverse the order of the merge arguments for the crdAccessRoles and
commonAnnotations to ensure correct precedence. Currently, commonAnnotations is
merged last (giving it higher priority), but it should be merged first with
crdAccessRoles merged afterward. This ensures that role-specific annotations in
crdAccessRoles override the common base annotations when keys conflict,
following the expected specific-over-general precedence pattern.
| "level": { | ||
| "type": "string", | ||
| "description": "The level to log at. One of 'debug', 'info', or 'error'.", | ||
| "default": "info" | ||
| }, | ||
| "format": { | ||
| "type": "string", | ||
| "description": "The log format to use. One of console, json.", | ||
| "default": "json" | ||
| }, | ||
| "stacktraceLevel": { | ||
| "type": "string", | ||
| "description": "The level to report stacktrace logs one of 'info' or 'error'.", | ||
| "default": "error" |
There was a problem hiding this comment.
Enforce documented allowed values with enum constraints.
These fields describe fixed allowed values but currently accept arbitrary strings, so invalid configs pass schema validation and fail later at runtime/behavior level.
Suggested fix
"level": {
"type": "string",
"description": "The level to log at. One of 'debug', 'info', or 'error'.",
+ "enum": ["debug", "info", "error"],
"default": "info"
},
"format": {
"type": "string",
"description": "The log format to use. One of console, json.",
+ "enum": ["console", "json"],
"default": "json"
},
"stacktraceLevel": {
"type": "string",
"description": "The level to report stacktrace logs one of 'info' or 'error'.",
+ "enum": ["info", "error"],
"default": "error"
}
...
"drainPolicy": {
"type": "string",
"description": "Policy for what to do with ngrok API resources while draining during an Uninstall. \"Delete\" removes ngrok API resources, \"Retain\" preserves them",
+ "enum": ["Delete", "Retain"],
"default": "Retain"
},
"defaultDomainReclaimPolicy": {
"type": "string",
"description": "The default domain reclaim policy to use for domains created by the operator. Valid values are \"Delete\" and \"Retain\". The default is \"Delete\".",
+ "enum": ["Delete", "Retain"],
"default": "Delete"
}Also applies to: 234-243
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@helm/ngrok-operator/values.schema.json` around lines 97 - 110, The schema
validation for the "level", "format", and "stacktraceLevel" fields documents
their allowed values in descriptions but does not enforce them with constraints,
allowing invalid configurations to pass validation. Add an `enum` constraint to
each field: for "level" add enum with values debug, info, and error; for
"format" add enum with values console and json; for "stacktraceLevel" add enum
with values info and error. Ensure these enum constraints match the allowed
values documented in each field's description. Also apply the same pattern to
the similar fields mentioned at lines 234-243.
| "maxUnavailable": { | ||
| "type": "string", | ||
| "description": "The name of the ServiceAccount to use for the agent.", | ||
| "default": "" | ||
| "description": "Maximum number/percentage of pods that may be made unavailable", | ||
| "default": "\"\"" | ||
| }, |
There was a problem hiding this comment.
Align schema default with chart default for apiManager.podDisruptionBudget.maxUnavailable.
Schema default is \"\", while helm/ngrok-operator/values.yaml defaults this to "1". This mismatch is already surfacing in generated docs and can mislead users.
Suggested fix
"maxUnavailable": {
"type": "string",
"description": "Maximum number/percentage of pods that may be made unavailable",
- "default": "\"\""
+ "default": "1"
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "maxUnavailable": { | |
| "type": "string", | |
| "description": "The name of the ServiceAccount to use for the agent.", | |
| "default": "" | |
| "description": "Maximum number/percentage of pods that may be made unavailable", | |
| "default": "\"\"" | |
| }, | |
| "maxUnavailable": { | |
| "type": "string", | |
| "description": "Maximum number/percentage of pods that may be made unavailable", | |
| "default": "1" | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@helm/ngrok-operator/values.schema.json` around lines 350 - 354, The schema
default for the maxUnavailable field within apiManager.podDisruptionBudget is
set to an empty string ("\"\""), but the actual default in values.yaml is "1".
Update the default value in the schema file for the maxUnavailable property to
match the chart's actual default of "1" to ensure consistency between the schema
documentation and the runtime configuration.
1 participant
What
Restructure the Helm values tree around shared platform config (ngrok.), feature flags (features.), and per-component sections (apiManager, agent, bindingsForwarder), implementing Option 2 of the helm values redesign.
App config now reaches the operator binaries the same way Argo CD does it: every app-config CLI flag reads an NGROK_OPERATOR_* environment variable as its default (CLI flag > env var > built-in default), and the chart injects those variables from per-component ConfigMaps via configMapKeyRef entries with optional: true. Per-component overrides are handled at render time by merging .config over the shared config into that component's ConfigMap. Pods roll on config changes via a checksum/config annotation.
Also:
How
Largely a claude prompt. Opening in draft until I can test it further.
Breaking Changes
Yes, largely around the helm values as we work towards 1.0. The helm chart attempts to guard against accidental breaking changes by failing if any of the old values are supplied instructing the user to update them
Summary by CodeRabbit
New Features
NGROK_OPERATOR_*environment variables with precedence: CLI flags > environment variables > built-in defaults.Configuration Changes
ngrok.*andfeatures.*; component-specific settings moved underapiManager,agent, andbindingsForwarder. Migration guide available.Documentation